Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StackedNotificationsBehavior port #170

Merged
merged 16 commits into from
Aug 11, 2023
Merged

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Aug 9, 2023

This migrates, updates, and amends the StackedNotificationsBehavior from Labs by @vgromfeld

This PR provides:

  • Initial port of the component into the Behaviors component
  • Update to initial sample to make clearer text is changing from notification to notification (cycle vs random)
  • Adds migration notes from the prior InAppNotification Toolkit component
  • Adds Show method alternates to help make migrating easier from the Toolkit component
  • Adds a Clear method to remove all Notifications
  • Add IconSource to Notification to allow for custom icons per notification
  • Adds more override logic for both Severity and Title, thus allowing parent properties on an InfoBar to be set and used by all notifications more easily
  • Adds a suite of tests for the common patterns/usages

Migration Notes from InAppNotification:

  • Missing StackMode to change behavior of queuing messages, was thinking of adding, but then realized it was a more disruptive user experience in general to replace an active notification. Added a note, thoughts @niels9001?
  • We don't provide an Opened/Opening event for the notification. Theoretically you should be able to just register to the IsOpen property changed notification... see related platform issue with InfoBar itself: Proposal: Add 'Opened' event to InfoBar microsoft/microsoft-ui-xaml#3761
  • Otherwise, I think we have a simpler and more straight-forward implementation here which covers the base Toolkit scenarios and should provide straight-forward migration. Feel free to check the new doc section during review.

Needs:

@michael-hawker michael-hawker added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 9, 2023
@michael-hawker
Copy link
Member Author

Force pushed, forgot to adjust author of initial port commit, did an interactive rebase and:

git commit --amend --author="Author Name email@address.com" --no-edit

@niels9001
Copy link
Collaborator

@michael-hawker updated the icon, made a minor tweak to the samples so they don't "jump" whenever a notification is shown using a MinHeight

@niels9001 niels9001 self-requested a review August 10, 2023 17:23
@michael-hawker
Copy link
Member Author

Seeing the tests failing with our unknown platform issue again, updating our build script with the diagnostic log flags to get more info (also updating some versions and other things needed by the CI that had diverted from Labs, like the Page file and non-.NET Native release).

@michael-hawker
Copy link
Member Author

With the extra diagnostics I did see there was an issue in the StackedNotificationsBehavior test due to the timer firing after the test had removed the UI element, so I fixed that by adding the StopTimer call in the Uninitialize method. Looks like we got a clean run there now, just waiting on WinUI 2 side. Though couldn't open the minidump from there due to mismatched .NET Core versions... 😟 Not sure how to resolve that to get the stack trace yet.

@michael-hawker
Copy link
Member Author

Added some extra build diagnostics to analyze crashes a bit in the pipeline itself (if they occur). Also trying to ignore the called out test and see what happens. 🤞

@Arlodotexe any concerns with the build modifications, think we can leave the diagnostics on all the time for now to help us, as the large dump only gets uploaded if there was a dump, so any good builds won't add much extra noise or size anyway?

@michael-hawker michael-hawker merged commit 10adbf7 into main Aug 11, 2023
7 checks passed
@delete-merged-branch delete-merged-branch bot deleted the llama/infobar-notifications branch August 11, 2023 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants